Extend BearerTokenCredentialPolicy to handle auth challenges #18437
Conversation
|
|
||
|
|
||
| class BearerTokenCredentialPolicy(_BearerTokenCredentialPolicyBase, SansIOHTTPPolicy): | ||
| class BearerTokenCredentialPolicy(_BearerTokenCredentialPolicyBase, SansIOHTTPPolicy, HTTPPolicy): |
There was a problem hiding this comment.
Then the pipeline will call on_request or send or both?
Do we have a design for this scenario? @annatisch
There was a problem hiding this comment.
After #16726, the pipeline will call this policy's send method (instead of _SansIOHTTPPolicyRunner.send). Another option is for this policy not to inherit SansIOHTTPPolicy.
There was a problem hiding this comment.
Since this is an "AvecIOHTTPPolicy" (as opposed to Sans), I would remove the inheritance from SansIOHTTPPolicy and derive from the "correct" base class. I think that your implementation can call self.on_request and self.on_response to make sure they are called in a very similar fashion as before in order to handle any cases where someone has extended/derived from the policy, no?
There was a problem hiding this comment.
Have done. The extended policy follows _SansIOHTTPPolicyRunner's calling pattern such that it calls a subclass's on_* as the runner would.
|
|
||
|
|
||
| class BearerTokenCredentialPolicy(_BearerTokenCredentialPolicyBase, SansIOHTTPPolicy): | ||
| class BearerTokenCredentialPolicy(_BearerTokenCredentialPolicyBase, HTTPPolicy): |
There was a problem hiding this comment.
I remember we planned to modify
if isinstance(policy, SansIOHTTPPolicy):to check hasattibute('on_request'), will that change conflict with this one?
There was a problem hiding this comment.
That's #16726. No conflict with this change, and this change doesn't require that PR.
| :param ~azure.core.pipeline.PipelineRequest request: the request | ||
| """ | ||
| self._enforce_https(request) |
There was a problem hiding this comment.
How do you think about moving _enforce_https into shared utils?
There was a problem hiding this comment.
Makes sense the next time another policy needs it. Do we want the key and SAS policies to enforce HTTPS as well?
There was a problem hiding this comment.
I asked this because I saw ACR also used it. :)
3 participants
When its on_challenge method is not overridden, the ChallengeAuthenticationPolicy proposed in #17489 is functionally equivalent to the existing BearerTokenCredentialPolicy. It's reasonable then to wonder how the existing policy would look when extended to handle authentication challenges. Short answer: it would look something like this 😉